feat: support ESLint v9 #2996
feat: support ESLint v9 #2996
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2996 +/- ##
==========================================
- Coverage 95.31% 95.14% -0.18%
==========================================
Files 82 82
Lines 3438 3439 +1
Branches 1186 1188 +2
==========================================
- Hits 3277 3272 -5
- Misses 161 167 +6 ☔ View full report in Codecov by Sentry. |
G-Rath
force-pushed
the
support-eslint-v9
branch
2 times, most recently
from
7c59670
to
543e378
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
I've just gotten back from travel, so I'll try to take a look at it in the coming days. One likely obstacle is that all the tests assume eslintrc, but eslint 9 requires an env var to support it, otherwise it defaults to flat config. The initial support needs to test eslintrc, and it would be fine to add flat config tests in a follow-on if needed. It's likely that the way eslint < 9's RuleTester works is subtly different than in eslint 9, so we may need an abstraction to handle that. |
|
Yup part of this includes switching to a custom rule tester that converts the tests from eslintrc to flat config if they're running on v9, which is what we've been using in |
|
That's great, but testing eslintrc on eslint 9 is also very important. It'd also be great if there was a commit or two I could land separately, that worked on eslint < 9, so that only the 9-specific parts remained in this PR after that was landed and rebased :-) |
|
If you're meaning the context helper stuff, yeah I'm happy to pull them out I just figured you'd have asked them to be tested against ESLint v9 to prove they actually worked :-) |
|
You figured right :-) but i can just cherry-pick the commits out of this PR once things are working. |
|
Right well they're already good for cherry-picking - you want to pick from e31fe5c...543e378 (sans f0853cb once #2997 is landed), and away you go. I think you can have the eslintrc-on-v9 support by just running the whole test suite twice with the env enabled and an extra test in the custom rule tester - I'll push that up shortly |
|
@ljharb |
|
well that sucks, we’ll need to file an eslint issue about that gap. |
|
I'll leave that to you as I suspect you'll have more pull :-) |
|
Filed eslint/eslint#18292. |
G-Rath
force-pushed
the
support-eslint-v9
branch
2 times, most recently
from
7d42925
to
f3318fb
Compare
G-Rath
force-pushed
the
support-eslint-v9
branch
from
5429dab
to
f1e7cd2
Compare
src/rules/newline-after-import.js
Outdated
| const scopeBody = getScopeBody(getScope(context, node)); | ||
| log('got scope:', scopeBody); |
There was a problem hiding this comment.
the scope is for the program, i think, so it should be gotten without the node, or with the entire program's node?
There was a problem hiding this comment.
Sweet, I'll try passing in the program node and see what happens
There was a problem hiding this comment.
i think this still needs addressing
| @@ -32,7 +32,7 @@ ruleTester.run('no-cycle', rule, { | |||
| test({ code: 'var foo = require("./")' }), | |||
| test({ code: 'var foo = require("@scope/foo")' }), | |||
| test({ code: 'var bar = require("./bar/index")' }), | |||
| test({ code: 'var bar = require("./bar")' }), | |||
| ...usingFlatConfig ? [] : [test({ code: 'var bar = require("./bar")' })], | |||
There was a problem hiding this comment.
why can't this one work in flat config?
There was a problem hiding this comment.
As it's considered a duplicate of the next line and v9 ruletester doesn't allow duplicate tests
There was a problem hiding this comment.
it's not, though - it has a different filename. v9 doesn't consider that to be different? sounds like a bug in v9.
There was a problem hiding this comment.
Yup, feel free to raise an issue over at eslint
There was a problem hiding this comment.
in that case, let's add a code comment or something so we can keep both test cases.
This comment was marked as outdated.
This comment was marked as outdated.
|
Totally, while it's still a draft I'm indeed just chipping away at the review :-) your effort is appreciated! |
G-Rath
force-pushed
the
support-eslint-v9
branch
from
055272b
to
4b7da9a
Compare
G-Rath
force-pushed
the
support-eslint-v9
branch
from
4b7da9a
to
eba7cb0
Compare
|
Alright, so I think I've tracked this down to That seems to happen because This means it's an existing bug in the flat config handling, which I can produce using v8. @ljharb would you be able to look into this and suggest some solutions? I'm not sure the best way to deal with it especially in a manner that preserves full backwards compatibility. |
Which build issue is this related to? The cases where the rule isn't reporting an error when it should, or these And is there possibly a correlation between those parsing errors and where you're seeing |
I'm working on
No, They're also happening on (it's because if the parser errors, then
Either I'm not following you or I don't think that's correct - from what I can see, technically you're right in saying "fallback" but the comments within |
That's actually not true. This one is from
They're happening on |
That's actually not true: The error in your screenshot is coming from
Also, I'm guessing you missed when I said that I could reproduce this using ESLint v8 on a simple reproduction, which also doesn't generate that parser error: I'm not saying it should be completely ignored, but from what I can tell it isn't the issue here - I do agree it's interesting that it doesn't seem to be happening everywhere those parsers are being used, but it does seems like its happening for dynamic import tests only and maybe that's only where this code path matters. |
|
So, I have a solution for addressing the function getBabelEslintVisitorKeys(parserPath) {
if (parserPath.endsWith('index.js')) {
const hypotheticalLocation = parserPath.replace('index.js', 'visitor-keys.js');
if (fs.existsSync(hypotheticalLocation)) {
const keys = moduleRequire(hypotheticalLocation);
return keys.default || keys;
}
}
return null;
}I guess we could try and use And @G-Rath I didn't miss anything you said, it's just that correlation isn't causation, and I'm trying to focus on identifying root causes. |
|
hmm, i guess that's a fair point that we don't actually have to support it with flat config, as long as we continue doing so for eslintrc. I'm fine with that as long as the error message is clear, and that tests are covering the behavior. |
|
Ok, I believe I've figured out the issue with testVersion('> 3', () => ({ // Dynamic import is not properly caracterized with eslint < 4
code: `import { foo } from "./${testDialect}/depth-one-dynamic"; // #2265 6`,
errors: [error(`Dependency cycle detected.`)],
parser: parsers.BABEL_OLD,
})),
test({
code: `import { foo } from "./${testDialect}/depth-one-dynamic"; // #2265 8`,
errors: [error(`Dependency cycle detected.`)],
parser: parsers.TS_NEW,
}),After some exploration, I found that the tests actually passed (correctly reported the error), if I didn't run the valid versions of the test which turns on test({
code: `import { foo } from "./${testDialect}/depth-one-dynamic"; // #2265 8`,
errors: [error(`Dependency cycle detected.`)],
parser: parsers.TS_NEW,
settings: {
'import/cache': {
lifetime: 0,
},
}
}),It's just a theory based on these explorations, but I think it could be pointing towards a root cause. Will explore more as I'm able. |
|
That's great to hear - a separate PR to fix the caching bug would be really helpful (and it's fine if tests aren't practical to write for that) |
What I haven't had time to check, though, is if this caching issue is being caused by how this PR wrap the v9 |
Quick update on this. It happens without the wrapped |
Resolves #2948